Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rules for substitute holidays to South Korea calendar #1707

Merged
merged 9 commits into from
Jun 20, 2023

Conversation

nistick21
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Jun 20, 2023

Coverage Status

coverage: 71.926% (+0.03%) from 71.9% when pulling 7c1e014 on nistick21:master into 0bc1e5e on lballabio:master.

Comment on lines 64 to 69
if ((w == Monday && (d == 2 || d == 3) && m == March && y > 2021) ||
(w == Monday && (d == 6 || d == 7) && m == May && y > 2013) ||
(w == Monday && (d == 16 || d == 17) && m == August && y > 2020) ||
(w == Monday && (d == 4 || d == 5) && m == October && y > 2020) ||
(w == Monday && (d == 10 || d == 11) && m == October && y > 2020) ||
(w == Monday && (d == 26 || d == 27) && m == December && y > 2022)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move each of these condition a bit below, together with the original condition for the corresponding holiday.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks better. thanks luigi.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I wasn't clear — I meant moving each substitute rule with its holiday. I pushed the change myself.
A suggestion for next time: it's better to open the PR from a branch, not master, so my pushes (if any) are less disruptive. Thanks!

@lballabio
Copy link
Owner

Thanks! May you add to the documentation in the hpp file a link to your source for the information?

@nistick21
Copy link
Contributor Author

I found a legal document about alternative public holidays in SouthKorea. Unfortunately, it is not written in English, but I will attach it to the header file.

@lballabio lballabio changed the title apply substitute holiday to southkorea calendar Add rules for substitute holidays to South Korea calendar Jun 20, 2023
@lballabio lballabio added this to the Release 1.31 milestone Jun 20, 2023
@nistick21
Copy link
Contributor Author

Your code is really awesome. Thanks for accept to 1.31 milestone.

@lballabio lballabio merged commit 97f7a2b into lballabio:master Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants